Repository navigation
fix: remove secrets from evault core - #413
Conversation
WalkthroughThe W3ID initialization logic is refactored to remove cryptographic key management and signing operations. Secret storage, nacl-based key handling, seed persistence, and signer creation are eliminated. The public API surface remains unchanged, but internal construction paths are simplified to directly build the W3IDBuilder instance. Changes
Sequence DiagramsequenceDiagram
participant Caller
participant W3ID
participant Builder
rect rgb(240, 248, 255)
note right of Caller: Previous Flow (Removed)
Caller->>W3ID: W3ID.get(id, driver, password)
W3ID->>W3ID: Load/Create Seed
W3ID->>W3ID: Create Signer from Seed
W3ID->>W3ID: Store Seed (persistence)
end
rect rgb(245, 255, 240)
note right of Caller: New Simplified Flow
Caller->>W3ID: W3ID.get(id, driver, password)
W3ID->>Builder: withId(id)
Builder->>Builder: withRepository(driver)
Builder->>Builder: withGlobal()
Builder-->>W3ID: Build Instance
W3ID-->>Caller: Return W3ID
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
infrastructure/evault-core/src/core/w3id/w3id.ts (1)
10-14: Remove the unusedpasswordparameter.The
passwordparameter is accepted but never used in the method body. This is misleading to API consumers who might pass a password expecting it to secure their W3ID instance.Either remove the parameter entirely or document it as deprecated if backward compatibility is required.
Apply this diff to remove the unused parameter:
static async get(options?: { id: string; driver: Driver; - password?: string; }) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
infrastructure/evault-core/src/core/w3id/w3id.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-16T18:04:28.983Z
Learnt from: coodos
Repo: MetaState-Prototype-Project/prototype PR: 99
File: infrastructure/w3id/tests/w3id.test.ts:48-57
Timestamp: 2025-04-16T18:04:28.983Z
Learning: In test files, suggestions to add defensive coding patterns (like extra null checks) are often unnecessary, as tests are designed to fail explicitly when expectations aren't met. The test in `infrastructure/w3id/tests/w3id.test.ts` validates that `id.logs` is an instance of `IDLogManager` before accessing its repository property.
Applied to files:
infrastructure/evault-core/src/core/w3id/w3id.ts
🧬 Code graph analysis (1)
infrastructure/evault-core/src/core/w3id/w3id.ts (2)
infrastructure/evault-core/src/core/w3id/log-service.ts (1)
LogService(8-42)infrastructure/w3id/src/w3id.ts (1)
W3IDBuilder(32-158)
🔇 Additional comments (2)
infrastructure/evault-core/src/core/w3id/w3id.ts (2)
1-2: LGTM: Type-only imports improve tree-shaking.The type-only imports for
DriverandW3IDare appropriate since they're only used for type annotations, reducing the runtime bundle size.
8-8: LGTM: Constructor simplified appropriately.The simplified private constructor is appropriate for the singleton pattern.
| const repository = new LogService(options.driver); | ||
| const keyId = `w3id-${options.id}`; | ||
|
|
||
| try { | ||
| // Try to get existing seed | ||
| const { seed, nextKeyHash } = await W3ID.secretsStore.getSeed(keyId); | ||
| const keyPair = nacl.sign.keyPair.fromSeed(seed); | ||
| W3ID.instance = await new W3IDBuilder() | ||
| .withId(options.id) | ||
| .withRepository(repository) | ||
| .withGlobal(true) | ||
| .withSigner(createSigner(keyPair)) | ||
| .withNextKeyHash(nextKeyHash) | ||
| .build(); | ||
| } catch { | ||
| // If no seed exists, create new one | ||
| const keyPair = nacl.sign.keyPair(); | ||
| const nextKeyPair = nacl.sign.keyPair(); | ||
| const nextKeyHash = await hash(uint8ArrayToHex(nextKeyPair.publicKey)); | ||
|
|
||
| // Store the seed | ||
| await W3ID.secretsStore.storeSeed(keyId, keyPair.secretKey, nextKeyHash); | ||
|
|
||
| W3ID.instance = await new W3IDBuilder() | ||
| .withId(options.id) | ||
| .withRepository(repository) | ||
| .withSigner(createSigner(keyPair)) | ||
| .withNextKeyHash(nextKeyHash) | ||
| .build(); | ||
| } | ||
| W3ID.instance = await new W3IDBuilder() | ||
| .withId(options.id) | ||
| .withRepository(repository) | ||
| .withGlobal(true) | ||
| .build(); |
There was a problem hiding this comment.
🧩 Analysis chain
Remove unused repository or add signer.
The LogService repository is created and passed to the builder, but according to the W3IDBuilder.build() logic (see relevant code snippets), when no signer is provided, it returns new W3ID(id) and completely ignores the repository.
This means:
- The Neo4j storage is unnecessarily initialized
- The code is misleading, suggesting log functionality exists when it doesn't
- Resources are wasted
Either remove the repository creation entirely, or this indicates incomplete refactoring where the signer should still be provided.
Apply this diff if logs functionality is truly no longer needed:
-const repository = new LogService(options.driver);
-
W3ID.instance = await new W3IDBuilder()
.withId(options.id)
- .withRepository(repository)
.withGlobal(true)
.build();Run the following script to verify if any callers expect the W3ID instance to have logs functionality:
🏁 Script executed:
#!/bin/bash
# Description: Check if any code attempts to access logs or signing functionality on W3ID instances
# Search for access to logs property or signing methods on W3ID instances
rg -n --type=ts -C3 '\bW3ID\b.*\.(logs|sign|verify|signer)'
# Search for IDLogManager usage that might depend on W3ID logs
rg -n --type=ts -C3 'IDLogManager'
# Check for any tests that verify signing/logging behavior
rg -n --type=ts -C3 'W3ID.*log|W3ID.*sign' -g '*test*' -g '*spec*'Length of output: 8651
---
🏁 Script executed:
```shell
#!/bin/bash
# Find the exact file referenced in the review
fd -type f -name "w3id.ts" | grep -E "(evault-core|w3id)" | head -20
# Also search for files containing LogService
rg -l "LogService" --type=ts | head -10
Length of output: 315
---
🏁 Script executed:
```shell
#!/bin/bash
# View the specific file and lines mentioned in the review
echo "=== File: infrastructure/evault-core/src/core/w3id/w3id.ts ==="
cat infrastructure/evault-core/src/core/w3id/w3id.ts
echo ""
echo "=== Also check log-service.ts ==="
cat infrastructure/evault-core/src/core/w3id/log-service.ts
Length of output: 1909
---
🏁 Script executed:
```shell
#!/bin/bash
# Find package.json for evault-core
echo "=== package.json in evault-core ==="
cat infrastructure/evault-core/package.json | grep -A 50 '"dependencies"'
# Check if w3id is a local package reference
echo ""
echo "=== Check workspaces or monorepo setup ==="
cat package.json 2>/dev/null | head -40
Length of output: 2859
Remove unused repository creation.
The LogService repository is created and passed to W3IDBuilder, but since no signer is provided, the builder's build() method returns new W3ID(id) without the logs parameter. This means the repository is never used.
From infrastructure/w3id/src/w3id.ts (the actual W3IDBuilder implementation in this monorepo), when no signer is provided, the builder ignores the repository entirely. The code wastefully initializes Neo4j storage that will never be accessed.
Remove the unused repository creation:
-const repository = new LogService(options.driver);
-
W3ID.instance = await new W3IDBuilder()
.withId(options.id)
- .withRepository(repository)
.withGlobal(true)
.build();If logging functionality is needed, add .withSigner() to the builder chain.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const repository = new LogService(options.driver); | |
| const keyId = `w3id-${options.id}`; | |
| try { | |
| // Try to get existing seed | |
| const { seed, nextKeyHash } = await W3ID.secretsStore.getSeed(keyId); | |
| const keyPair = nacl.sign.keyPair.fromSeed(seed); | |
| W3ID.instance = await new W3IDBuilder() | |
| .withId(options.id) | |
| .withRepository(repository) | |
| .withGlobal(true) | |
| .withSigner(createSigner(keyPair)) | |
| .withNextKeyHash(nextKeyHash) | |
| .build(); | |
| } catch { | |
| // If no seed exists, create new one | |
| const keyPair = nacl.sign.keyPair(); | |
| const nextKeyPair = nacl.sign.keyPair(); | |
| const nextKeyHash = await hash(uint8ArrayToHex(nextKeyPair.publicKey)); | |
| // Store the seed | |
| await W3ID.secretsStore.storeSeed(keyId, keyPair.secretKey, nextKeyHash); | |
| W3ID.instance = await new W3IDBuilder() | |
| .withId(options.id) | |
| .withRepository(repository) | |
| .withSigner(createSigner(keyPair)) | |
| .withNextKeyHash(nextKeyHash) | |
| .build(); | |
| } | |
| W3ID.instance = await new W3IDBuilder() | |
| .withId(options.id) | |
| .withRepository(repository) | |
| .withGlobal(true) | |
| .build(); | |
| W3ID.instance = await new W3IDBuilder() | |
| .withId(options.id) | |
| .withGlobal(true) | |
| .build(); |
🤖 Prompt for AI Agents
in infrastructure/evault-core/src/core/w3id/w3id.ts around lines 21 to 27, the
code instantiates LogService and passes it to W3IDBuilder but the builder
returns a plain W3ID when no signer is provided so the repository is never used;
remove the unused repository creation and the .withRepository(...) call (and any
related unused imports), or if you actually need logging functionality, supply a
signer by adding .withSigner(...) to the builder chain so the repository will be
consumed.
3 participants
Description of change
removes secrets dependency in evault core
Issue Number
n/a
Type of change
How the change has been tested
ci checks
Change checklist
Summary by CodeRabbit